Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FragPipe #761

Merged
merged 34 commits into from
Jul 10, 2024
Merged

Add FragPipe #761

merged 34 commits into from
Jul 10, 2024

Conversation

reid-wagner
Copy link
Contributor

Re-creating #759

This PR adds a wrapper for the FragPipe tool suite.

A handful of default FragPipe workflows are provided for the user, while more may be added in the future. Most FragPipe parameters are duplicated in the Galaxy tool, many of which are optional to allow the workflow defaults to be used.

Some components have not yet been implemented due to license restrictions (DIA-NN & MSBooster). Other parameters need further work and testing before implementation, and will default to the workflow defaults. The workflow, along with the log file and some other FragPipe outputs are optionally selected as Galaxy outputs.

License keys are hard-coded into the tool, and a required select option and text warnings are in place to ensure a user understands the licenses, as has been discussed with the developers.

@bgruening
Copy link
Member

You can now see the planemo error reports at this page: https://github.com/galaxyproteomics/tools-galaxyp/actions/runs/9046215197?pr=761 scroll down.

Seems like the mv command has nothing to move. How do you run the tests locally?

@reid-wagner
Copy link
Contributor Author

Yes - it looks like libz is a dependency that might need to be added: libz.so.1: cannot open shared object file. It was already present on our system so I didn't catch it.

When I try to add it to the recipe with <requirement type="package" version="1.2.13">zlib</requirement>, the FragPipe package doesn't seem to be installed correctly (i.e. can't find fragpipe binary on path), so I haven't figured that out.

I'll add zlib to the bioconda recipe anyways, but I was hope to get it working here to verify that's all we need. I'm trying to run tests with planemo test --engine docker_galaxy --no_cache_galaxy, but I'm having separate issues there too. I am able to run this manually on our university Galaxy instance.

@bgruening
Copy link
Member

When I try to add it to the recipe with <requirement type="package" version="1.2.13">zlib</requirement>, the FragPipe package doesn't seem to be installed correctly (i.e. can't find fragpipe binary on path), so I haven't figured that out.

In those cases its probably the wrong, incompatible zlib version. Try on your commandline conda create -n foo --dry-run fragpipe=x.x.x zlib=y.y.y you will see that the solver will bail out maybe and then you can play around with the version until it works.

But you are correct, fixing the conda recipe is the correct thing to do.

I'll add zlib to the bioconda recipe anyways, but I was hope to get it working here to verify that's all we need. I'm trying to run tests with planemo test --engine docker_galaxy --no_cache_galaxy, but I'm having separate issues there too. I am able to run this manually on our university Galaxy instance.

Oh I see, this is wrong :)

Just do planemo test --biocontainers this will run your tests using a container (that is created if needed). And it is exactly what is running here on CI.

@reid-wagner
Copy link
Contributor Author

you can play around with the version until it works

This sort-of how I came to zlib=1.2.13. I built the fragpipe recipe with an unpinned zlib dependency and used the resulting zlib version. Creating a new environment with zlib and fragpipe as separate dependencies also resolved to 1.2.13. I'm wondering if there's a way to find logs of the mulled environment creation?

Oh I see, this is wrong :)

Okay, thanks for the advice! I'm just getting started with planemo test so I have a lot to learn.

@reid-wagner
Copy link
Contributor Author

@bgruening Is there a way to force builds to use a biocontainer with a more recent build of the fragpipe bioconda package? I fixed the zlib issue in the recipe, since I had trouble with it here, but I think it's still using the older version.

@reid-wagner
Copy link
Contributor Author

@bgruening Is there a way to force builds to use a biocontainer with a more recent build of the fragpipe bioconda package? I fixed the zlib issue in the recipe, since I had trouble with it here, but I think it's still using the older version.

Apologies - I didn't sufficiently fix the dependency. And it is using the most recent biocontainer.

@bgruening
Copy link
Member

Are you saying the _3 container here: https://quay.io/repository/biocontainers/fragpipe?tab=tags&tag=latest is not working?

1 similar comment
@bgruening
Copy link
Member

Are you saying the _3 container here: https://quay.io/repository/biocontainers/fragpipe?tab=tags&tag=latest is not working?

@reid-wagner
Copy link
Contributor Author

reid-wagner commented Jun 13, 2024

It's not, but I made an obvious mistake with the dependencies so I'm working to fix that in the recipe now.

Never mind, the dependency fix was fine.

@reid-wagner reid-wagner marked this pull request as ready for review June 14, 2024 16:02
@reid-wagner reid-wagner removed the wip label Jun 14, 2024
@reid-wagner
Copy link
Contributor Author

This is ready for review now.

There actually was an issue with the biocontainer - it seems like ldconfig is not appropriately setting the library path in the container. I worked around this by adding /usr/local/lib to LD_LIBRARY_PATH in the recipe.

Copy link
Collaborator

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial thoughts .... maybe we can sit together these days and see if we can simplify the configfile generation or just go through step by step... But not sure when.

tools/fragpipe/fragpipe.xml Outdated Show resolved Hide resolved
tools/fragpipe/fragpipe.xml Outdated Show resolved Hide resolved
tools/fragpipe/fragpipe.xml Outdated Show resolved Hide resolved
tools/fragpipe/msfragger_macros.xml Outdated Show resolved Hide resolved
tools/fragpipe/genericize_db.py Outdated Show resolved Hide resolved
tools/fragpipe/macros.xml Outdated Show resolved Hide resolved
tools/fragpipe/fragpipe.xml Outdated Show resolved Hide resolved
tools/fragpipe/fragpipe.xml Outdated Show resolved Hide resolved
tools/fragpipe/macros.xml Outdated Show resolved Hide resolved
tools/fragpipe/fragpipe.xml Outdated Show resolved Hide resolved
@reid-wagner reid-wagner force-pushed the add_fragpipe branch 4 times, most recently from cf595a1 to 6ebe238 Compare June 29, 2024 19:01
… FragPipe creates cache relative to the lib location, this allows a local FragPipe cache.
tools/fragpipe/macros.xml Outdated Show resolved Hide resolved
tools/fragpipe/macros.xml Outdated Show resolved Hide resolved
tools/fragpipe/macros.xml Outdated Show resolved Hide resolved
tools/fragpipe/macros.xml Outdated Show resolved Hide resolved
tools/fragpipe/macros.xml Show resolved Hide resolved
tools/fragpipe/macros.xml Outdated Show resolved Hide resolved
tools/fragpipe/fragpipe.xml Show resolved Hide resolved
<when value="default"/>
<when value="no"/>
<when value="yes">
<section name="options" expanded="true" title="Isobaric Quantification">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does having a single section make sense here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little confused on this point. Would it be to remove the "no" and "default" options? The "yes"/"no" distinction allows us to set the param that enables this in FragPipe. The "default" is usually a bit redundant but the argument could be that if a user is re-running a shared job, they might not know the workflow default and could use that option to revert to it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default / no / yes is fine for me. was just wondering if a single section inside a conditional is helpful.

but fine with me.

tools/fragpipe/macros.xml Show resolved Hide resolved
tools/fragpipe/macros.xml Show resolved Hide resolved
Copy link
Collaborator

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good to me. Any comments @bgruening ?

@reid-wagner
Copy link
Contributor Author

@bernt-matthias Thanks for the review!

…nd use_glycan_composition accessed incorrectly.
@bgruening bgruening merged commit 905cc2b into galaxyproteomics:master Jul 10, 2024
11 checks passed
@bgruening
Copy link
Member

Awesome! Thanks @reid-wagner and @bernt-matthias

@reid-wagner
Copy link
Contributor Author

Thank you @bgruening and @bernt-matthias!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants